Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cli): archive list option to filter by name prefix. Fixes #10524 #13794

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tooptoop4
Copy link
Contributor

Fixes #10524

@agilgur5 agilgur5 changed the title fix(cli): archive list option to filter by name prefix (#10524) feat(cli): archive list option to filter by name prefix. Fixes #10524 Oct 21, 2024
if err != nil {
return err
}
return printer.PrintWorkflows(workflows, os.Stdout, printer.PrintOpts{Output: output.String(), Namespace: true, UID: true})
},
}
command.Flags().VarP(&output, "output", "o", "Output format. "+output.Usage())
command.Flags().StringVar(&prefix, "prefix", "", "Filter workflows by prefix")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this should be more specific like "name prefix"; the API parameter is NamePrefix too, so it would be good to match

Suggested change
command.Flags().StringVar(&prefix, "prefix", "", "Filter workflows by prefix")
command.Flags().StringVar(&prefix, "name-prefix", "", "Filter workflows by name prefix")

Copy link

@agilgur5 agilgur5 Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also make sense to consider the UX/style of #13160, where --name can be modified by --name-filter, which is one of "Exact" / "Prefix" / "Pattern".

Although it didn't implement that for the Archived List API specifically, but I think that API is capable of both "Exact" and "Prefix" already, so we can make the CLI syntax/UX match the newer List API

Copy link

@agilgur5 agilgur5 Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see --prefix and the description verbatim match the list CLI, so perhaps we leave that in for backward-compat but also add --name and --name-filter to match #13160.

We could also remove it in a breaking CLI change or just only add the new variant to the archive list command so that there's nothing to deprecate later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

argo archive list needs a filter on wfname
2 participants